Added port validation #12233

kadern0 · 2021-08-11T07:52:11Z

Tests:

minikube start --driver=docker --ports=asdf:80 --ports=443:443
😄  minikube v1.22.0 on Ubuntu 20.04
✨  Using the docker driver based on existing profile

❌  Exiting due to MK_USAGE: Sorry, one of the ports provided with --ports flag is not valid [asdf:80 443:443]

minikube start --driver=docker --ports=8080:80 --ports=443:443
😄  minikube v1.22.0 on Ubuntu 20.04
✨  Using the docker driver based on existing profile

❌  Exiting due to MK_USAGE: Sorry, you cannot use privileged ports on the host (below 1024) [8080:80 443:443]

minikube start --driver=docker --ports=8080:80 --ports=6443:443
😄  minikube v1.22.0 on Ubuntu 20.04
✨  Using the docker driver based on existing profile
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
🔄  Restarting existing docker container for "minikube" ...
🐳  Preparing Kubernetes v1.21.3 on Docker 20.10.7 ...
🔎  Verifying Kubernetes components...
    ▪ Using image k8s.gcr.io/ingress-nginx/controller:v0.44.0
    ▪ Using image docker.io/jettech/kube-webhook-certgen:v1.5.1
    ▪ Using image docker.io/jettech/kube-webhook-certgen:v1.5.1
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🔎  Verifying ingress addon...
🌟  Enabled addons: storage-provisioner, default-storageclass, ingress

🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

k8s-ci-robot · 2021-08-11T07:52:19Z

Hi @kadern0. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

minikube-bot · 2021-08-11T07:57:05Z

Can one of the admins verify this patch?

cmd/minikube/cmd/start.go

spowelljr · 2021-08-11T23:54:24Z

Thanks for the validation @kadern0, it looks good! If I could ask you to do one more thing, if you wouldn't mind writing a test for this validation in start_test.go.

If you call

minikube/cmd/minikube/cmd/start_flags.go

Line 252 in a70bdb5

    
           func generateClusterConfig(cmd *cobra.Command, existing *config.ClusterConfig, k8sVersion string, drvName string) (config.ClusterConfig, config.Node, error) {

in the test it will run flag validation, there are other tests in start_test.go that do this exact same thing if you're looking for an example.

Thanks!

kadern0 · 2021-08-12T23:05:52Z

@spowelljr thanks for the feedback!
Adding tests for this function might not be as straightforward (for me, at least) as it seems. Unlike the other tests on the file, this function doesn't return anything (same as the other validation functions) and unlike the other two tests that call 'generateClusterConfig', we can't test the function from there since this function is called from 'runStart'. So unless I'm missing anything, options are: no test (as with most of validating functions), refactor and add a return statement and test the function directly (although this isn't being done for the other validation functions), making a call to the function during the configuration generation so we can test it from there.
What do you think?

cmd/minikube/cmd/start.go

medyagh · 2021-08-13T00:52:07Z

@spowelljr thanks for the feedback!
Adding tests for this function might not be as straightforward (for me, at least) as it seems. Unlike the other tests on the file, this function doesn't return anything (same as the other validation functions) and unlike the other two tests that call 'generateClusterConfig', we can't test the function from there since this function is called from 'runStart'. So unless I'm missing anything, options are: no test (as with most of validating functions), refactor and add a return statement and test the function directly (although this isn't being done for the other validation functions), making a call to the function during the configuration generation so we can test it from there.
What do you think?

how about adding a unit test in start_test.go
just for this func

validatePorts(ports []string)

kadern0 · 2021-08-13T01:02:27Z

@medyagh so the plan is: refactoring the function so it returns error and then create a unit test for it, right?

medyagh · 2021-08-13T20:31:26Z

@medyagh so the plan is: refactoring the function so it returns error and then create a unit test for it, right?

Yes please . Thank you for your helpful prs

kadern0 · 2021-08-14T05:51:15Z

@medyagh I've made the changes as suggested but now it seems the cyclomatic complexity of the function validateFlags is > 30. It seems it might require additional changes.

kadern0 · 2021-08-14T05:59:39Z

Maybe these could be made functions in order to reduce the complexity of validateFlags:

https://github.com/kubernetes/minikube/blob/c3c4d0455dfed89650fdf54f9f70d551912b4969/cmd/minikube/cmd/start.go#L1090:L1099
https://github.com/kubernetes/minikube/blob/c3c4d0455dfed89650fdf54f9f70d551912b4969/cmd/minikube/cmd/start.go#L1121:L1145
https://github.com/kubernetes/minikube/blob/c3c4d0455dfed89650fdf54f9f70d551912b4969/cmd/minikube/cmd/start.go#L1147:L1164

What do you think?

kadern0 · 2021-09-19T22:59:56Z

@medyagh , @spowelljr I think this PR is ready for another review :)

spowelljr · 2021-09-20T16:39:48Z

@kadern0 The code looks good! One last thing, could you run make generate-docs and push the changes up and then we'll be good to go, thanks for your patience.

cmd/minikube/cmd/start.go

k8s-ci-robot · 2021-09-27T22:53:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadern0, spowelljr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [spowelljr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 11, 2021

k8s-ci-robot requested review from blueelvis and spowelljr August 11, 2021 07:52

spowelljr reviewed Aug 11, 2021

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

kadern0 force-pushed the issue-10495 branch 2 times, most recently from 0777f14 to e19c734 Compare August 11, 2021 22:28

medyagh requested changes Aug 13, 2021

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

Added port validation

1f83aa3

kadern0 force-pushed the issue-10495 branch from e19c734 to 1f83aa3 Compare August 14, 2021 05:49

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 14, 2021

kadern0 requested a review from medyagh August 14, 2021 05:51

Refactored code into functions. Added tests

69a5a93

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2021

kadern0 requested a review from spowelljr August 31, 2021 00:06

spowelljr requested changes Sep 20, 2021

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

spowelljr requested changes Sep 27, 2021

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

Fixed error exit. Added docs

4e1bcc0

kadern0 force-pushed the issue-10495 branch from ec2d479 to 4e1bcc0 Compare September 27, 2021 22:44

spowelljr approved these changes Sep 27, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2021

spowelljr merged commit f85e7db into kubernetes:master Sep 27, 2021

mans0954 mentioned this pull request Nov 10, 2021

Port validation breaks options that used to work on Linux #12919

Closed

tyabu12 mentioned this pull request Mar 18, 2022

Port validation error on specifying tcp/udp or range of ports #13809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added port validation #12233

Added port validation #12233

kadern0 commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

minikube-bot commented Aug 11, 2021

spowelljr commented Aug 11, 2021

kadern0 commented Aug 12, 2021

medyagh commented Aug 13, 2021

kadern0 commented Aug 13, 2021

medyagh commented Aug 13, 2021

kadern0 commented Aug 14, 2021

kadern0 commented Aug 14, 2021

kadern0 commented Sep 19, 2021

spowelljr commented Sep 20, 2021

k8s-ci-robot commented Sep 27, 2021

Added port validation #12233

Added port validation #12233

Conversation

kadern0 commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

minikube-bot commented Aug 11, 2021

spowelljr commented Aug 11, 2021

kadern0 commented Aug 12, 2021

medyagh commented Aug 13, 2021

kadern0 commented Aug 13, 2021

medyagh commented Aug 13, 2021

kadern0 commented Aug 14, 2021

kadern0 commented Aug 14, 2021

kadern0 commented Sep 19, 2021

spowelljr commented Sep 20, 2021

k8s-ci-robot commented Sep 27, 2021